-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Convert CumeDist to UDWF #13051
Conversation
CUME_DIST() OVER(ORDER BY c9) as cd1, | ||
CUME_DIST() OVER(ORDER BY c9 ROWS BETWEEN 10 PRECEDING and 1 FOLLOWING) as cd2 | ||
cume_dist() OVER(ORDER BY c9) as cd1, | ||
cume_dist() OVER(ORDER BY c9 ROWS BETWEEN 10 PRECEDING and 1 FOLLOWING) as cd2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is consistent with conversion of other user-defined window functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@jonathanc-n A roundtrip test for datafusion/datafusion/proto/tests/cases/roundtrip_logical_plan.rs Lines 943 to 952 in c22abb4
|
@@ -940,6 +940,7 @@ async fn roundtrip_expr_api() -> Result<()> { | |||
vec![lit(1), lit(2), lit(3)], | |||
vec![lit(10), lit(20), lit(30)], | |||
), | |||
cume_dist(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
- [dense_rank](#dense_rank) | ||
- [percent_rank](#percent_rank) | ||
- [rank](#rank) | ||
- [row_number](#row_number) | ||
|
||
### `cume_dist` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Love it -- thank you @jonathanc-n and @findepi @timsaucer and @jcsherin for the reviews |
🚀 |
Which issue does this PR close?
Closes #12695 .
Rationale for this change
What changes are included in this PR?
Converted CumeDist to user defined window function.
Are these changes tested?
Are there any user-facing changes?